Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(*): add support for new architecture #67

Merged
merged 28 commits into from
May 2, 2024

Conversation

ThibaultBee
Copy link
Member

Update to Fabric architecture and clean the project

@ThibaultBee
Copy link
Member Author

ThibaultBee commented Aug 23, 2023

Status: Waiting for test

What has been done?

  • iOS: both old and new arch
  • Android: both old and new arch
  • Android: manage permissions at low level (bugfix in progress)
  • Android preview: [Bug]: LiveStreamView Warp #64

Not tested but it should fixed

What is missing?

  • tests Android legacy arch
  • tests Android new arch
  • tests iOS legacy arch
  • tests iOS new arch

TODO

  • Issue on orientation when changing device from portrait to landscape

How can I help?

  • By testing the example with one or several of the following case: Android old arch, Android new arch, iOS old arch, iOS new arch.

How to test?

Please write you have been testing 🙏

@ThibaultBee ThibaultBee mentioned this pull request Aug 23, 2023
2 tasks
@ThibaultBee ThibaultBee force-pushed the feature/new_arch_support branch 7 times, most recently from a474326 to c9390cf Compare December 18, 2023 09:20
@ThibaultBee ThibaultBee force-pushed the feature/new_arch_support branch 4 times, most recently from ee24a7c to e38608b Compare December 21, 2023 14:44
@ThibaultBee ThibaultBee force-pushed the feature/new_arch_support branch 6 times, most recently from 0f2cd62 to 340f3b5 Compare January 4, 2024 10:25
@ThibaultBee ThibaultBee force-pushed the feature/new_arch_support branch 4 times, most recently from 5455f91 to 3b6e344 Compare January 4, 2024 16:09
@ThibaultBee
Copy link
Member Author

@BlueBazze

Your issue happens here: https://github.com/ThibaultBee/StreamPack/blob/994b995705e1566ccb2a18d848ad907e4afaebdb/core/src/main/java/io/github/thibaultbee/streampack/internal/sources/AudioSource.kt#L63
I guess we should check that create returns a null object. But it is odd that isAvailable is true in this case. I will make the change but this will be released later.

I am currently releasing the Android dependency to integrate the fixes on the preview in this branch.

Are you going to perform other tests?
Not sure if you said it but have you tested on new arch or on old arch?

@Veryinheart @matthewfleming How long do you need to test this PR?

@BlueBazze
Copy link
Contributor

BlueBazze commented Mar 1, 2024

Are you going to perform other tests?

Yes, i havent started actually streaming yet.

Not sure if you said it but have you tested on new arch or on old arch?

The very first one was with the old arch i believe.
But it should be the new one. I specified expo to use the new arch: https://github.com/BlueBazze/api.video-reactnative-live-stream-expo-demo/blob/master/app.json#L45-L50
But i dont know how it works under the hood and how it knows which arch to use from the package.

Is there a way to check which arch it is running on from the running code?

@ThibaultBee
Copy link
Member Author

But i dont know how it works under the hood and how it knows which arch to use from the package.

This package is supposed to support both arch on iOS and Android.

Is there a way to check which arch it is running on from the running code?

Not sure... There might be a log somewhere.

@Veryinheart
Copy link

@BlueBazze

Your issue happens here: https://github.com/ThibaultBee/StreamPack/blob/994b995705e1566ccb2a18d848ad907e4afaebdb/core/src/main/java/io/github/thibaultbee/streampack/internal/sources/AudioSource.kt#L63 I guess we should check that create returns a null object. But it is odd that isAvailable is true in this case. I will make the change but this will be released later.

I am currently releasing the Android dependency to integrate the fixes on the preview in this branch.

Are you going to perform other tests? Not sure if you said it but have you tested on new arch or on old arch?

@Veryinheart @matthewfleming How long do you need to test this PR?

Hi @ThibaultBee , we have tested on some devices and below is the result:

IOS:
iPhone 14 Pro - iOS 16.2 works well
iPhone X - iOS 15.5 works well
iPhone 7 -iOS 14.7. works well

Android:
Samsung galaxy S9 - android 10 works well. Live streaming dropped once and restarted it and everything is ok, so might be the network issue.
Samsung galaxy S7 - works well
Pixel 6a - android 14- works well

Question: Do we support live stream in background mode? live stream will be stopped if we move app to background mode when it is live streaming.

@ThibaultBee
Copy link
Member Author

ThibaultBee commented Mar 5, 2024

Question: Do we support live stream in background mode? live stream will be stopped if we move app to background mode when it is live streaming.

Yes, this is on purpose. I thought using cameras in background was not legit for Android but I am not sure anymore. But we will have to use a backgroud service and it is not something we want to do.
Moreover, it is the same behavior as camera applications.

@BlueBazze
Copy link
Contributor

I've tested streaming on Iphone 8 and Samsung A22, couldn't find any problems.
Havent tested orientation change, ill get to it as soon as possible.

@BlueBazze
Copy link
Contributor

Yes, this is on purpose. I thought using cameras in background was not legit for Android but I am not sure anymore. But we will have to use a backgroud service and it is not something we want to do. Moreover, it is the same behavior as camera applications.

When i looked into this, the easiest way was to use PiP.
Back then i found this https://github.com/adkaushik/react-native-pip-android
But havent seen any other packages allowing this functionality, and i've thought of it as too big a hassle.
So if it creates another component specifically for the pip view, then react would render that as a seperate native component, and it would not contain the same data and state as the original stream view. Causing the pip view not to be live.
Otherwise the entire stream functionality would have to be packed into a singleton on the native side and each stream view would just be using the singleton information. Meaning only one stream could be active at any one point.

But yes. Older people (and even some younger) will not know why their stream stopped just because they tapped away from the application. What i have done is to display an in-app notification telling the user their stream has stopped, even if they stop streaming themselves..

@BlueBazze
Copy link
Contributor

BlueBazze commented Mar 6, 2024

Orientation

Only tested on android.
The phone was rotated while the app was running before going live.
image

I restarted the app in landscape.
image

@ThibaultBee
Copy link
Member Author

That's going to be a tricky one.

If you remove orientation in android:configChanges that might fix the issue.
On native Android application, the activity is recreated when the orientation of the device changes. This is not the case in RN application (not sure why).
But this is definitely something that should be handle by the library.

@BlueBazze
Copy link
Contributor

BlueBazze commented Mar 8, 2024

If you remove orientation in android:configChanges that might fix the issue.

I dont believe it is present. With default expo the only app config available is https://docs.expo.dev/versions/latest/config/app/#orientation

Just to add on the orientation.
It works fine with the current master version as far as i know.

Listening to orientation change with an event listener and keep a state with the current orientation.
When pressing go live, lock the app orientation to the current.
When going offline you unlock the orientation.

This has worked great so far. But if the outgoing stream is not oriented according to the preview the user would not know until looking at the viewer perspective.

@ThibaultBee
Copy link
Member Author

ThibaultBee commented Mar 8, 2024

Your orientation setting is not the same as orientation in configChanges.
See

android:configChanges="keyboard|keyboardHidden|orientation|screenSize|uiMode"

I need to dig a bit deeper in this issue.

@soemarko
Copy link

I need to dig a bit deeper in this issue.

I don't think this is an odd ask, but why not release this with the bug and continue working on the fix?

I don't think this bug has anything to do with this feature?

@ThibaultBee
Copy link
Member Author

Mainly because this regression is due to the new architecture, so it is related to this branch.
Then, I think because of this regression, the lib is not usable. And the developer might not see the issue till it own application is in production.

And... the fix is going to be postponed because I am focusing on api.video project with a higher priority.

@ThibaultBee
Copy link
Member Author

@BlueBazze
This issue was really a pain.
I just made a workaround for now. Could you give it a try?

@nikitapilgrim
Copy link

nikitapilgrim commented Apr 25, 2024

@ThibaultBee
for some reason the preview is black, sometimes when remount the component it appears. (iOS) legacy

@BlueBazze
Copy link
Contributor

@BlueBazze
This issue was really a pain.
I just made a workaround for now. Could you give it a try?

Sorry, I kept forgetting about it.
I'll test first thing tomorrow morning.

@ThibaultBee
Copy link
Member Author

ThibaultBee commented Apr 25, 2024

@ThibaultBee for some reason the preview is black, sometimes when remount the component it appears. (iOS) legacy

@nikitapilgrim
I don't reproduce your issue. Are you using the provided example? How can I reproduced this behavior easily?

@BlueBazze
Copy link
Contributor

Done testing on android. The previous issue has not occurred.
I believe that i tested the rotation on iOS last month. Ill test it again to make sure.

@BlueBazze
Copy link
Contributor

Notice that,

Start streaming in portrait
Stop streaming
Rotate phone to landscape
Start streaming
After a few seconds it automatically throws onDisconnect

Log from the example app

 LOG  Streaming started
 LOG  Received onConnectionSuccess
 LOG  Received onDisconnect

This is because of my streaming streaming server not being able to handle a resolution change during the reconnection.
The reconnection window is about 1 minute.
Tried replicating this issue in OBS which also just kept retrying the connection.

The end user could accidentally start the stream in portrait, and then after seeing the preview change it to landscape.

The easiest solution would be to implement a timer when this scenario occurs.

I know this cant be fixed from the package. But down the line, it would be nice to see a reason argument for onDisconnect, that would describe how the stream was disconnected.

Was the disconnect userInitiated, connectionClosed etc..

@nikitapilgrim
Copy link

@ThibaultBee
#67 (comment)

I use react-native-vision-camera and this module in my project. When switching from react-native-vision-camera to this module, this bug occurs.
I was able to solve the problem by not displaying 'ApiVideoLiveStreamView' 500ms in timeout.

@ThibaultBee
Copy link
Member Author

ThibaultBee commented May 2, 2024

hmm looks like the camera has not been released when this package tries to access it. If this is the case, we can't really do something on our side.
Could you provide the logcat and/or a sample application to reproduce it?

@ThibaultBee
Copy link
Member Author

@BlueBazze
Please open an issue for this. Do you see any returns except connection closed by remote ou connection closed by user

@nikitapilgrim
Could you also open an issue so we can track it?

I don't want to hold the release for these but we can work on it in a second time.

@ThibaultBee ThibaultBee merged commit 86df97a into main May 2, 2024
7 checks passed
@ThibaultBee ThibaultBee deleted the feature/new_arch_support branch May 2, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants